Redesign procedure 2PC skip: static analysis via PLpgSQL plugin#8566
Redesign procedure 2PC skip: static analysis via PLpgSQL plugin#8566codeforall wants to merge 5 commits into
Conversation
Replace the runtime counter approach from PR #8524 with pre-execution static analysis of procedure bodies using the PLpgSQL plugin func_beg hook. This avoids partial commits followed by ERROR that the original design could produce for multi-statement procedures. Key changes: - New procedure_body_analysis.c: PLpgSQL plugin that walks the statement tree at func_beg time, counting SQL-producing statements (EXECSQL, PERFORM, DYNEXECUTE, CALL) and detecting disqualifiers (COMMIT, ROLLBACK, and all loop types). - Loops (FOR, WHILE, LOOP, FOREACH) are treated as disqualifiers since they can execute SQL multiple times, risking partial commits. - Multi-statement procedures fall back gracefully to normal coordinated transactions (2PC) instead of raising an ERROR. - Remove ProcedureNonCoordinatedExecutionCount global counter and its three reset sites in utility_hook.c. - Update tests Replace the runtime counter approach from PR #8524 with pre-execution static analysis of procedure bodies using the PLpgSQ. Adds a temporary benchmarking script, for testing/dev purposes only (will not be in final commit)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8566 +/- ##
==========================================
- Coverage 88.73% 88.69% -0.05%
==========================================
Files 288 289 +1
Lines 64428 64538 +110
Branches 8113 8125 +12
==========================================
+ Hits 57170 57241 +71
- Misses 4915 4948 +33
- Partials 2343 2349 +6 🚀 New features to boost your workflow:
|
| } | ||
| } | ||
|
|
||
| count += WalkStatementList(if_stmt->else_body, disqualified); |
There was a problem hiding this comment.
Would alternate branches of an IF statement return a count of 2 ? This, for example:
IF do_insert
THEN INSERT INTO t VALUES (x);
ELSE UPDATE t SET val = x WHERE key = y; -- count = 2 after seeing this statement ?
END IF;
If so would something like return count + max( Walk (when), Walk(else) ) ensure only one branch is accounted for ? Nbd if not, take this as a nice-to-have suggestion.
There was a problem hiding this comment.
Done!. WalkStatementList now contributes the maximum SQL count across IF branches (then/ELSIF/else) and CASE branches (WHEN/else) instead of the sum, since only one branch runs at runtime. So an IF/CASE with one SQL statement per branch is now correctly single-statement and eligible for the skip. Safety is preserved: a branch that by itself holds ≥2 statements still counts as multi, a disqualifier (loop/COMMIT/ROLLBACK) in any branch still disqualifies the whole body via *disqualified, and sibling statements outside the IF/CASE still accumulate (sum). A BLOCK still sums its body + exception-handler action (both may run). Added TEST 23 (if_one_per_branch) and TEST 25 (case_one_per_branch) as positive skip cases, both guard-verified: reverting max→sum flips them from skip to coordinated.
| * executes and set the ProcedureBodyIsSingleStatement flag. | ||
| * | ||
| * The analysis recursively walks the PLpgSQL statement tree counting | ||
| * SQL-producing statements (EXECSQL, PERFORM, DYNEXECUTE) and detecting |
There was a problem hiding this comment.
let's add CALL to this list, as thats the use-case that motivated this feature
There was a problem hiding this comment.
Done!
PLPGSQL_STMT_CALL is counted alongside EXECSQL/PERFORM/DYNEXECUTE, and the file header/contract comments were updated to reflect it. (A single-CALL procedure is now an eligible single-statement case.)
| return count; | ||
| } | ||
| } | ||
| count += WalkStatementList(case_stmt->else_stmts, disqualified); |
There was a problem hiding this comment.
Any reason not to also short-circuit after walking the else branch ?
There was a problem hiding this comment.
Addressed!. Each branch walk is now followed by an explicit if (*disqualified) return count;, and after count += branchMax the top-of-loop if (*disqualified || count > 1) return count; check short-circuits the remainder,
so the walk no longer continues needlessly after the else branch resolves the count/disqualifier.
| { | ||
| *disqualified = true; | ||
| return count; | ||
| } |
There was a problem hiding this comment.
Should it allow loop bodies with 0 SQL statements ? As it stands, this procedure body would be disqualified right?
FOR i IN 1..n LOOP
total := total + i; -- no SQL statements in this loop
END LOOP;
INSERT INTO t VALUES (total); -- Total SQL statement count = 1
Nbd if additional logic make this awkward, its outside of the core use-case for the feature.
There was a problem hiding this comment.
Can we keep the unconditional loop disqualifier for now? I've documented the rationale in the loop-case comment: safely relaxing it would require proving a loop body is SQL-free at every nesting depth, the pattern is rare, and the conservative baseline only costs a missed optimization, never correctness. Left as a possible future enhancement. Added (loop_in_if) to pin that a loop nested inside an IF branch still disqualifies the whole procedure.
| "Multi-statement procedures gracefully fall back to the " | ||
| "normal coordinated transaction path."), | ||
| &EnableProcedureTransactionSkip, | ||
| false, |
There was a problem hiding this comment.
This looks like a positive change overall, let's enable the GUC by default to see impact on existing CI tests, and even consider removing the GUC and having this be the default behavior for procedure calls.
There was a problem hiding this comment.
Kept the GUC defaulting off for this PR, we can do it as a separate follow up after deciding between enabling this by default or possibly removing the GUC entirely
Count the maximum number of SQL statements across IF/CASE branches instead of their sum, so a procedure with a single statement per branch stays eligible for the single-statement transaction skip. Reset the single-statement flag on the procedure error path so it never survives a failed procedure, and document why loops are disqualified and why the feature stays behind an off-by-default GUC. Add regression coverage for the IF, CASE, nested-block, and exception walker paths, and remove the temporary benchmark script.
Move plpgsql.h into its own group after postgres.h so the file passes the sort-and-group-includes style check.
3 participants
DESCRIPTION: Avoid coordinated transactions for single-statement single-shard procedure calls
Replace the runtime-counter approach from PR#8524 with pre-execution static analysis of procedure bodies using the PLpgSQL plugin func_beg hook. The prior design detected multi-statement procedures only after the first statement had run, which could produce a partial commit followed by an ERROR. Analyzing the body before any statement executes lets multi-statement procedures fall back cleanly to a coordinated (2PC) transaction with no error and no partial commit.
How it works
A new PLpgSQL plugin walks the statement tree at func_beg time, before any statement runs and sets the ProcedureBodyIsSingleStatement flag. The executor gate (CanSkipProcedureCoordination) then treats a single-statement body as necessary but not sufficient: the existing single-task / single-placement / coordinated-transaction checks still apply before a skip is allowed.
The walker:
Counts SQL-producing statements: EXECSQL, PERFORM, DYNEXECUTE, and CALL (the motivating use case).
Disqualifies on COMMIT, ROLLBACK, and every loop type (LOOP, WHILE, FOR/FORI/FORS/FORC, DYNFORS, FOREACH), a statement inside a loop can run many times, which is exactly the partial-commit risk we avoid.
Recurses into container statements: a BLOCK contributes the sum of its body and its exception-handler actions (both may run), while an IF or CASE contributes the maximum SQL count across its branches (then/ELSIF/else, or WHEN/else) because only one branch executes at runtime. A disqualifier in any branch still disqualifies the whole body.
Key changes